Skip to content

Adding open file location option in notification - #283

Merged
Gigas002 merged 5 commits into
waycrate:mainfrom
Suryansh-Dey:open_location
Apr 4, 2026
Merged

Adding open file location option in notification#283
Gigas002 merged 5 commits into
waycrate:mainfrom
Suryansh-Dey:open_location

Conversation

@Suryansh-Dey

@Suryansh-Dey Suryansh-Dey commented Feb 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Feature

Now you can click the notification to open file location. Similar to windows. So you can easily rename, move, delete or view it.

Reason of kernel fork

I did try tokio::spawn but issue is child processes get killed when parent process exits.
Now I may disown the thread but in many cases it creates different issues due to parent getting killed and was too error prone to handle.

This might look scary at first glance but is a very safe way to do it.
Ready for you feedback.

@Suryansh-Dey

Suryansh-Dey commented Feb 5, 2026

Copy link
Copy Markdown
Contributor Author

This PR feel a bit unnecessary to me after raising this pr😅 Rest is maintainers choice.

@Decodetalkers

Decodetalkers commented Feb 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

This PR feel a bit unnecessary to me after raising this pr😅 Rest is maintainers choice.

I would prefer this pr first, because I am considering to replace the dir library. But first I want to hear from other maintainers @Gigas002 @Shinyzenith what do you think about this pr?

@Gigas002 Gigas002 mentioned this pull request Feb 6, 2026
Draft
@Gigas002 Gigas002 mentioned this pull request Apr 1, 2026
Closed
@Gigas002

Gigas002 commented Apr 2, 2026

Copy link
Copy Markdown
Member

@Suryansh-Dey
Sorry for the wait. After some thinking and seeing requests for better notifications customizations I think I prefer this over #285.
Could you please rebase the branch? We've done some major refactoring and hidden notification behind the feature and separated some code.

@codecov-commenter

codecov-commenter commented Apr 3, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 47 lines in your changes missing coverage. Please review.
✅ Project coverage is 17.01%. Comparing base (4b99b3a) to head (a06d16c).

Files with missing lines Patch % Lines
wayshot/src/notification.rs 0.00% 40 Missing ⚠️
wayshot/src/wayshot.rs 0.00% 5 Missing ⚠️
wayshot/src/config.rs 0.00% 1 Missing ⚠️
wayshot/src/settings.rs 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #283      +/-   ##
==========================================
- Coverage   17.23%   17.01%   -0.23%     
==========================================
  Files          20       20              
  Lines        3057     3098      +41     
==========================================
  Hits          527      527              
- Misses       2530     2571      +41
Flag Coverage Δ
libwayshot 22.04% <ø> (ø)
wayshot 0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Suryansh-Dey

Suryansh-Dey commented Apr 3, 2026

Copy link
Copy Markdown
Contributor Author

Done, now people can click the notification and open the saved folder. Since most file explorer shows the most recent image on top, user can rename etc. on it

Gigas002
Gigas002 reviewed Apr 3, 2026
View reviewed changes
Comment thread wayshot/src/notification.rs
@Suryansh-Dey

Suryansh-Dey commented Apr 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I don't know why clippy is raising this error in CI (It don't in my neovim) error: method "get_action" is never used because get_action was used in src/settings.rs. May I know what is the purpose of --no-default-features flag?

@Gigas002

Gigas002 commented Apr 3, 2026

Copy link
Copy Markdown
Member

cargo clippy --all-features looks good, --no-default-features run fails.
It allows compiling wayshot without all default features (minimal working setup with minimal feature set). notification gets disabled in that case, and that raises warning.

@Suryansh-Dey

Suryansh-Dey commented Apr 3, 2026

Copy link
Copy Markdown
Contributor Author

cargo clippy --all-features looks good, --no-default-features run fails. It allows compiling wayshot without all default features (minimal working setup with minimal feature set). notification gets disabled in that case, and that raises warning.

What should I do then?

@Gigas002

Gigas002 commented Apr 3, 2026

Copy link
Copy Markdown
Member

Gating NotificationConfig behind #[cfg(feature = "notifications")] should help. You can run cargo clippy --no-default-features on your local environment to test.

@Suryansh-Dey

Suryansh-Dey commented Apr 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Done, Thank you for help. I actually I thought --no-default-feature is a flag to clippy binary to be minimal but actually it meant for our crate features. Sorry for inconvenience.

@Gigas002

Gigas002 commented Apr 3, 2026

Copy link
Copy Markdown
Member

@Suryansh-Dey
Sorry for the wait, finally got home to my machine and tested your PR.
I've fixed some stuff, sorry I couldn't explain better in my messages: I wanted action to be xdg-open by default and made it run any user-provided action. And since we can handle any command, I removed Click to see location message from notification.
@Decodetalkers can you please check this PR as well? it should be good as of now

@Gigas002
Gigas002 requested a review from Decodetalkers April 3, 2026 15:14
Decodetalkers
Decodetalkers approved these changes Apr 4, 2026
View reviewed changes

@Decodetalkers Decodetalkers left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it works well, and the modification is also good, so LGTM

@Gigas002
Gigas002 merged commit 6c4c8db into waycrate:main Apr 4, 2026
10 checks passed
@Gigas002

Gigas002 commented Apr 4, 2026

Copy link
Copy Markdown
Member

@Suryansh-Dey
Thanks for contributing! Merged into main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Gigas002 Gigas002 Gigas002 left review comments
@Decodetalkers Decodetalkers Decodetalkers approved these changes
@Shinyzenith Shinyzenith Awaiting requested review from Shinyzenith

Assignees

No one assigned

Labels

component: wayshot-cli enhancement New feature or request

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Suryansh-Dey @Decodetalkers @Gigas002 @codecov-commenter